Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: set-namespace without kyaml/rnode dependencies #769

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Mar 8, 2022

This function re-implements the set-namespace function but removes all kyaml dependencies.
It's not ready to be merged until its dependent functions are merged in kpt-functions-sdk

@yuwenma yuwenma force-pushed the set-namespace-v2 branch 2 times, most recently from faf138b to 02329db Compare March 16, 2022 05:49
@yuwenma yuwenma marked this pull request as ready for review March 16, 2022 05:49
@yuwenma yuwenma force-pushed the set-namespace-v2 branch 6 times, most recently from 1bbbcb0 to 7a862ab Compare March 16, 2022 19:52
functions/go/Makefile Outdated Show resolved Hide resolved
@yuwenma yuwenma force-pushed the set-namespace-v2 branch 3 times, most recently from 30b96e0 to c115f50 Compare March 18, 2022 20:32
examples/set-namespace-simple/README.md Outdated Show resolved Hide resolved
examples/set-namespace-simple/README.md Outdated Show resolved Hide resolved
examples/set-namespace-simple/README.md Show resolved Hide resolved
functions/go/set-namespace/README.md Outdated Show resolved Hide resolved
functions/go/set-namespace/README.md Show resolved Hide resolved
var subjects []*Subject
o.GetOrDie(&subjects, "subjects")
for _, s := range subjects {
if s.Name == "default" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in this doc, there are 2 scenarios to update the namespace field:

  • subject is a ServiceAccount and its name is "default"
  • subject is a ServiceAccount and its name is not "default", but there is a ServiceAccount object that has the matching name.

It's a superset of what you currently have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, do you mind me making the change in the following PR. I have some other logic relevant to this need to add as well. Basically, the new set-namespace only expect a single namespace exists. It raises error if multiple namespace is detected. Thus, here the namespace will be updated if and only if it is a ServiceAccount kind and the existing namespace matches the found one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with doing this logic in a followup PR.

@yuwenma yuwenma force-pushed the set-namespace-v2 branch 2 times, most recently from 701d255 to 6efd22e Compare March 22, 2022 14:04
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@mengqiy
Copy link
Contributor

mengqiy commented Mar 22, 2022

@mengqiy
Copy link
Contributor

mengqiy commented Mar 22, 2022

I split it out in #783.
When #783 merges, this PR can be merged with a rebase.

@mengqiy
Copy link
Contributor

mengqiy commented Mar 22, 2022

@yuwenma you can rebase this PR, it should be passing after that.

@yuwenma yuwenma force-pushed the set-namespace-v2 branch 2 times, most recently from 062d7d0 to 01efe36 Compare March 23, 2022 02:22
@yuwenma yuwenma merged commit 3a66116 into GoogleContainerTools:master Mar 23, 2022
@yuwenma yuwenma deleted the set-namespace-v2 branch March 23, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants